Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

events: handle inherited properties properly #2350

Closed
wants to merge 1 commit into from
Closed

events: handle inherited properties properly #2350

wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

As of now, the events module considers inherited properties as one of the
valid types, by default.

> process.version
'v3.0.0'
> events.EventEmitter.listenerCount(new events.EventEmitter(), 'toString')
1

This patch makes sure that the inherited properties are considered as
normal types and they will not be counted unless explicitly added.

> process.version
'v4.0.0-pre'
> events.EventEmitter.listenerCount(new events.EventEmitter(), 'toString')
0
> const emitter = new events.EventEmitter();
undefined
> emitter.on('toString', function() {});
EventEmitter {
  domain:
   Domain {
     domain: null,
     _events: { error: [Function] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] },
  _events: { toString: [Function] },
  _eventsCount: 1,
  _maxListeners: undefined }
> events.EventEmitter.listenerCount(emitter, 'toString')
1

I am guessing this would be a semver-major change, as this could be a breaking change.

@thefourtheye thefourtheye added events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 11, 2015
@thefourtheye thefourtheye mentioned this pull request Aug 11, 2015
Closed
@vkurchatkin
Copy link
Contributor

nodejs/node-v0.x-archive#4366

@thefourtheye
Copy link
Contributor Author

Oh. Performance hit. Okay, how about using a Map instead?

@vkurchatkin
Copy link
Contributor

#1785

@brendanashworth
Copy link
Contributor

Could someone run benchmarks on the entire new EventEmitter()?

The event emitter code is considered a red hot code path and obj = Object.create(null) is 20-30 times slower than obj = {}.

This doesn't seem to be benchmarking much, obj = {} could be a thousandth of the actual time spent creating the EE.

brendanashworth
brendanashworth reviewed Aug 13, 2015
View reviewed changes
test/parallel/test-event-emitter-listeners-side-effects.js
@@ -23,7 +23,7 @@ assert(fl.length === 1);
assert(fl[0] === assert.fail);

e.listeners('bar');
assert(!e._events.hasOwnProperty('bar'));
assert(!e._events['bar']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a necessary change?

@thefourtheye
Copy link
Contributor Author

Closing in favour of #1785

@thefourtheye thefourtheye deleted the fix-for-event-listener-and-inherited-properties branch August 15, 2015 22:59
@thefourtheye thefourtheye mentioned this pull request Oct 22, 2015
Closed
@thefourtheye thefourtheye mentioned this pull request Nov 14, 2015
Closed
@thefourtheye thefourtheye restored the fix-for-event-listener-and-inherited-properties branch November 15, 2015 04:58
@thefourtheye
Copy link
Contributor Author

Reopening this thread as #1785 was shot down and Object.create is ten times slower than Object literal but the difference is a very small number. The script I used is at #1785 (comment) cc @nodejs/collaborators

@thefourtheye thefourtheye reopened this Nov 15, 2015
@thefourtheye
events: handle inherited properties properly
85b2939 
As of now, the events module considers inherited properties as one of the
valid types, by default.

    > process.version
    'v3.0.0'
    > events.EventEmitter.listenerCount(new events.EventEmitter(), 'toString')
    1

This patch makes sure that the inherited properties are considered as
normal types and they will not be counted unless explicitly added.

    > process.version
    'v4.0.0-pre'
    > events.EventEmitter.listenerCount(new events.EventEmitter(), 'toString')
    0
    > const emitter = new events.EventEmitter();
    undefined
    > emitter.on('toString', function() {});
    EventEmitter {
      domain:
       Domain {
         domain: null,
         _events: { error: [Function] },
         _eventsCount: 1,
         _maxListeners: undefined,
         members: [] },
      _events: { toString: [Function] },
      _eventsCount: 1,
      _maxListeners: undefined }
    > events.EventEmitter.listenerCount(emitter, 'toString')
    1
@thefourtheye thefourtheye force-pushed the fix-for-event-listener-and-inherited-properties branch from a18b67e to 85b2939 Compare November 15, 2015 05:07
@mscdex
Copy link
Contributor

mscdex commented Nov 16, 2015

@thefourtheye I'm not seeing the same results that you described in that other issue. Here is what I get with current master and your benchmark script:

Object.create: 7359.860ms
Object Literal: 695.927ms

@targos
Copy link
Member

targos commented Nov 16, 2015

I'm seeing the same kind of benchmark results but I'm not sure we can trust them. V8 may be smart enough to see that we don't do anything with the object literal and not create it at all.

@targos
Copy link
Member

targos commented Nov 16, 2015

@thefourtheye can you compare the EE benchmarks between master and master + this PR ?

@mscdex
Copy link
Contributor

mscdex commented Nov 16, 2015

@targos I just added code that interacts with obj in both scenarios and I still see the same results.

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 22, 2016
@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@thefourtheye ... is this still something you want to pursue?

@thefourtheye
Copy link
Contributor Author

@jasnell Yes, as this has to be fixed somehow. I'll close this now and open a separate PR.

@thefourtheye thefourtheye deleted the fix-for-event-listener-and-inherited-properties branch March 25, 2016 09:55
@thefourtheye thefourtheye mentioned this pull request Apr 7, 2016
Merged
3 tasks
@snyk-bot snyk-bot mentioned this pull request Jan 16, 2022
Open
This was referenced Jan 16, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Jan 31, 2022
Open
@snyk-bot snyk-bot mentioned this pull request Jun 24, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@thefourtheye @vkurchatkin @brendanashworth @mscdex @targos @jasnell